WIP: Add partials to support circular references by alganet · Pull Request #36 · alganet/apywire

alganet · 2025-12-18T23:41:23Z

⚠️ WORK IN PROGRESS ⚠️

Implements allow_partial opt-in feature that permits specs with circular dependencies to be constructed.
Move circular dependency detection when allow_partial=false to WiringBase constructor.
Initial PoC
Remove and stash unrelated tests added just to reach 95% coverage
Reach 95% coverage by covering the new features
~~Refactoring~~ (will be dealt with in a separate PR)
~~Docs~~ (will be dealt with in a separate PR)

Copilot

Pull request overview

This work-in-progress PR introduces support for partial construction to handle circular dependencies in the wiring system. The implementation allows objects with circular references to be instantiated by creating skeleton placeholders that are later finalized.

Key Changes:

New allow_partial parameter enables opt-in partial construction for circular dependencies
Skeleton placeholder mechanism using _apywire_partial, _apywire_event, and _apywire_failure attributes
Early cycle detection during container initialization using topological sorting (Kahn's algorithm)

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 24 comments.

Show a summary per file

File	Description
`apywire/partial.py`	New module implementing compiled partial construction helper
`apywire/runtime.py`	Adds skeleton creation, finalization, and waiting logic for runtime partial construction
`apywire/wiring.py`	Implements early cycle detection with unified dependency graph analysis
`apywire/compiler.py`	Integrates partial construction support into compiled code generation
`apywire/threads.py`	Updates cache checking to wait for partial skeletons to finalize
`apywire/exceptions.py`	Adds new exception types for partial construction errors
`tests/test_partial_helper.py`	Comprehensive tests for partial construction helper logic
`tests/test_graph_detection.py`	Tests for cycle detection in dependency graphs
`tests/test_compile_partial.py`	Tests for compiled partial construction behavior
`tests/test_edge_cases.py`	Integration tests demonstrating skeleton identity preservation
`tests/test_threading.py`	Updates threading tests for new cycle detection behavior
`tests/test_single.py`	Updates single-threaded tests for early cycle detection
`tests/test_constant_placeholders.py`	Updates constant placeholder cycle detection tests
`tests/test_cli.py`	Additional CLI error handling tests
`apywire/__main__.py`	Adds pragma comment to CLI entrypoint
`Makefile`	Removes `reuse` dependency from format target
`uv.lock.license`	Adds license header for lock file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-19T02:46:15Z

+                        setattr(cls, "__new__", _tmp_new)
+                        try:
+                            ret = constructor(*pos_args, **kwargs_dict)
+                        finally:
+                            # Restore original __new__
+                            if orig_new is None:
+                                try:
+                                    delattr(cls, "__new__")
+                                except Exception:
+                                    pass
+                            else:
+                                try:
+                                    setattr(cls, "__new__", orig_new)
+                                except Exception:
+                                    pass


The temporary override of cls.__new__ (lines 380-394) is not thread-safe. In multi-threaded environments, if two threads are simultaneously resolving the same circular dependency, they could both attempt to modify cls.__new__ at the same time, leading to race conditions. One thread might restore the original __new__ while another thread is still using the temporary version, causing unpredictable behavior.

Copilot · 2025-12-19T02:46:15Z

+                    setattr(expected_cls, "__new__", _tmp_new)
+                    try:
+                        ret: object = constructor_callable()
+                    finally:
+                        if orig_new is None:
+                            try:
+                                delattr(expected_cls, "__new__")
+                            except Exception:  # pragma: no cover - defensive
+                                pass
+                        else:
+                            try:
+                                setattr(expected_cls, "__new__", orig_new)
+                            except Exception:
+                                pass


The temporary override of expected_cls.__new__ is not thread-safe. Multiple threads could simultaneously modify this class attribute, leading to race conditions where one thread's temporary __new__ is used by another thread, or where the restoration happens at the wrong time. This is a critical issue in multi-threaded environments.

Copilot · 2025-12-19T02:46:15Z

+                            exc = PartialConstructionError(
+                                "failed to initialize skeleton for %r: %s"
+                                % (name, e)


The format string uses old-style % formatting instead of f-strings, which is inconsistent with the f-string formatting used on line 437 in the same function. For consistency and modern Python best practices, this should use f-string formatting.

Copilot · 2025-12-19T02:46:16Z

+    # `_find_wiredref_names` removed — unused.



The comment on line 418 states "_find_wiredref_names removed — unused" but there is no corresponding removal in the diff. This comment appears to be inaccurate or misleading since the method being referenced doesn't appear to exist in the shown diff context.

Suggested change

# `_find_wiredref_names` removed — unused.

Copilot · 2025-12-19T02:46:16Z

+# Duplicate `_WiredRef` definition removed — the class is defined above near
+# the top of the module to keep related logic together.


The comment on line 522-523 states "Duplicate _WiredRef definition removed" but there is no corresponding removal in the diff. This comment appears to be documenting code changes that are not visible in this diff, which could be confusing for reviewers.

Suggested change

# Duplicate `_WiredRef` definition removed — the class is defined above near

# the top of the module to keep related logic together.

# `_WiredRef` is defined near the top of the module to keep related logic together.

Copilot · 2025-12-19T02:46:20Z

+                        else:
+                            try:
+                                setattr(expected_cls, "__new__", orig_new)
+                            except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

except Exception: # pragma: no cover - defensive

# Best-effort cleanup: ignore failure to restore __new__

Copilot · 2025-12-19T02:46:21Z

+                                except Exception:
+                                    pass
+                            else:
+                                try:
+                                    setattr(cls, "__new__", orig_new)
+                                except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

pass

else:

try:

setattr(cls, "__new__", orig_new)

except Exception:

except Exception:

# Best-effort cleanup: failure to remove __new__ is non-fatal

pass

else:

try:

setattr(cls, "__new__", orig_new)

except Exception:

# Best-effort cleanup: failure to restore __new__ is non-fatal

Copilot · 2025-12-19T02:46:21Z

+                                except Exception:
+                                    pass
+                            else:
+                                try:
+                                    setattr(cls, "__new__", orig_new)
+                                except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

pass

else:

try:

setattr(cls, "__new__", orig_new)

except Exception:

except Exception:

# Best-effort cleanup: if we cannot delete the temporary

# __new__ attribute, ignore the error to avoid masking

# the original construction exception.

pass

else:

try:

setattr(cls, "__new__", orig_new)

except Exception:

# Best-effort cleanup: if we cannot restore the original

# __new__ method, ignore the error to avoid masking the

# original construction exception.

Copilot · 2025-12-19T02:46:21Z

+                        if orig_new is None:
+                            try:
+                                delattr(expected_cls, "__new__")
+                            except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

except Exception: # Defensive: best-effort cleanup; ignore failures restoring __new__

Copilot · 2025-12-19T02:46:21Z

+                        else:
+                            try:
+                                setattr(expected_cls, "__new__", orig_new)
+                            except Exception:


'except' clause does nothing but pass and there is no explanatory comment.

Suggested change

except Exception:

except Exception:

# Best-effort restoration of __new__; if this fails,

# we keep the temporary __new__ and still propagate

# the original construction error to the caller.

alganet · 2025-12-19T02:52:39Z

The __new__ thread unsafety caught me by surprise. While it could be avoided somehow, I think it's better to look for other way to support partials.

Also, this PR got too large too fast. I'll make another one that just focuses on the circular dependency check on the WiringBase constructor.

Add partials to support circular references

3220823

alganet force-pushed the partials branch from f3a40e6 to 3220823 Compare December 19, 2025 02:35

alganet requested a review from Copilot December 19, 2025 02:37

Copilot started reviewing on behalf of alganet December 19, 2025 02:37 View session

Copilot AI reviewed Dec 19, 2025

View reviewed changes

alganet closed this Dec 19, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add partials to support circular references #36

WIP: Add partials to support circular references #36
alganet wants to merge 1 commit into
mainfrom
partials

alganet commented Dec 18, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

Copilot AI Dec 19, 2025

Uh oh!

alganet commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		# Duplicate `_WiredRef` definition removed — the class is defined above near
		# the top of the module to keep related logic together.

	# Duplicate `_WiredRef` definition removed — the class is defined above near
	# the top of the module to keep related logic together.
	# `_WiredRef` is defined near the top of the module to keep related logic together.

	except Exception:
	except Exception: # pragma: no cover - defensive
	# Best-effort cleanup: ignore failure to restore __new__

	except Exception:
	except Exception: # Defensive: best-effort cleanup; ignore failures restoring __new__

-                            except Exception:
+                            except Exception:
+                                # Best-effort restoration of __new__; if this fails,
+                                # we keep the temporary __new__ and still propagate
+                                # the original construction error to the caller.

Conversation

alganet commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

alganet commented Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alganet commented Dec 18, 2025 •

edited

Loading